-
Notifications
You must be signed in to change notification settings - Fork 82
trsm is not actually implemented yet #2814
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2814 +/- ##
==========================================
- Coverage 67.79% 67.79% -0.01%
==========================================
Files 58 58
Lines 20723 20723
==========================================
- Hits 14050 14049 -1
- Misses 6673 6674 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Benchmark Results
Benchmark PlotsA plot of the benchmark results has been uploaded as an artifact at https://github.com/EnzymeAD/Enzyme.jl/actions/runs/19671170824/artifacts/4673611025. |
|
incidentally that was actually reverted: JuliaLang/LinearAlgebra.jl#1239 but apparently not backported yet |
| "syrk", | ||
| "trmm", | ||
| "trsm", | ||
| # "trsm", Not actually implemented yet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry quick post commit review, can this be limited to 1.12+.
It will otherwise slow down other code by making it non-blas, if not differentiated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof, that's frustrating, shouldn't we ideally not do the blas replacement when a function is not being differentiated? Otherwise this problem occurs for many more functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does indeed occur for more functions (which is why we should be somewhat sparing on the blas replacements).
The issue is that at the time of replacement, we haven't run activity analysis so we don't know what is needed and what isn't
Partial fix to #2813
The crux is that Julia used a different function and while
trsmis defined in Enzyme proper there aren't currently any rules attached https://github.com/EnzymeAD/Enzyme/blob/a13f632e2fbaed1f971de015f15e8f4b353e66cb/enzyme/Enzyme/BlasDerivatives.td#L705-L713